[PATCH] mmc: mmci: Do not release spinlock in request_end

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Ulf Hansson]

The patch "mmc: core: move ->request() call from atomic context",
is the reason to why this change is possible. This simplifies the
error handling code execution path quite a lot and potentially also
fixes some error handling hang problems.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
---
 drivers/mmc/host/mmci.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 40e4c05..f7755c9 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -165,13 +165,7 @@ mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 	host->mrq = NULL;
 	host->cmd = NULL;
 
-	/*
-	 * Need to drop the host lock here; mmc_request_done may call
-	 * back into the driver...
-	 */
-	spin_unlock(&host->lock);
 	mmc_request_done(host->mmc, mrq);
-	spin_lock(&host->lock);
 }
 
 static void mmci_set_mask1(struct mmci_host *host, unsigned int mask)
-- 
1.7.5.4

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Linus Walleij]

On Tue, Oct 11, 2011 at 4:06 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:

> The patch "mmc: core: move ->request() call from atomic context",
> is the reason to why this change is possible. This simplifies the
> error handling code execution path quite a lot and potentially also
> fixes some error handling hang problems.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>

Adrian do you agree with this?

To me it looks correct,
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Adrian Hunter]

On 12/10/11 12:41, Linus Walleij wrote:
> On Tue, Oct 11, 2011 at 4:06 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> 
>> The patch "mmc: core: move ->request() call from atomic context",
>> is the reason to why this change is possible. This simplifies the
>> error handling code execution path quite a lot and potentially also
>> fixes some error handling hang problems.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> 
> Adrian do you agree with this?

Yes

> 
> To me it looks correct,
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Russell King - ARM Linux]

On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
> The patch "mmc: core: move ->request() call from atomic context",
> is the reason to why this change is possible. This simplifies the
> error handling code execution path quite a lot and potentially also
> fixes some error handling hang problems.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>

This doesn't look right:

void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
{
        if (err && cmd->retries) {
                host->ops->request(host, mrq);

So, not dropping the spinlock results in calling the request function
with the spinlock held - and as the request function then goes on to
lock the spinlock, we will deadlock.

I don't see anything in current mainline which addresses this, so I'm
not able to queue this patch.

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Jon Medhurst (Tixy)]

On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
> > The patch "mmc: core: move ->request() call from atomic context",
> > is the reason to why this change is possible. This simplifies the
> > error handling code execution path quite a lot and potentially also
> > fixes some error handling hang problems.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> 
> This doesn't look right:
> 
> void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
> {
>         if (err && cmd->retries) {
>                 host->ops->request(host, mrq);
> 
> So, not dropping the spinlock results in calling the request function
> with the spinlock held - and as the request function then goes on to
> lock the spinlock, we will deadlock.

Indeed, deadlock behaviour at this point is what I see with this patch
on a Versatile Express board running 3.0-rc9.

-- 
Tixy

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Ulf Hansson]

Jon Medhurst (Tixy) wrote:
> On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
>> On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
>>> The patch "mmc: core: move ->request() call from atomic context",
>>> is the reason to why this change is possible. This simplifies the
>>> error handling code execution path quite a lot and potentially also
>>> fixes some error handling hang problems.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
>> This doesn't look right:
>>
>> void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>> {
>>         if (err && cmd->retries) {
>>                 host->ops->request(host, mrq);
>>

This is NOT how it looks at mmc-next. You need to test with Adrian 
Hunters patch which the commit refers two.

Linux next for mmc is available at:

git://dev.laptop.org/users/cjb/mmc mmc-next



>> So, not dropping the spinlock results in calling the request function
>> with the spinlock held - and as the request function then goes on to
>> lock the spinlock, we will deadlock.
> 
> Indeed, deadlock behaviour at this point is what I see with this patch
> on a Versatile Express board running 3.0-rc9.
> 

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Russell King - ARM Linux]

On Fri, Oct 14, 2011 at 09:37:51AM +0200, Ulf Hansson wrote:
> Jon Medhurst (Tixy) wrote:
>> On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
>>> On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
>>>> The patch "mmc: core: move ->request() call from atomic context",
>>>> is the reason to why this change is possible. This simplifies the
>>>> error handling code execution path quite a lot and potentially also
>>>> fixes some error handling hang problems.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
>>> This doesn't look right:
>>>
>>> void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>>> {
>>>         if (err && cmd->retries) {
>>>                 host->ops->request(host, mrq);
>>>
>
> This is NOT how it looks at mmc-next. You need to test with Adrian  
> Hunters patch which the commit refers two.

In that case, how can I take the patch to mmci if it depends on something
in another tree?

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Fri, Oct 14, 2011 at 09:37:51AM +0200, Ulf Hansson wrote:
>> Jon Medhurst (Tixy) wrote:
>>> On Thu, 2011-10-13 at 15:29 +0100, Russell King - ARM Linux wrote:
>>>> On Tue, Oct 11, 2011 at 04:06:41PM +0200, Ulf Hansson wrote:
>>>>> The patch "mmc: core: move ->request() call from atomic context",
>>>>> is the reason to why this change is possible. This simplifies the
>>>>> error handling code execution path quite a lot and potentially also
>>>>> fixes some error handling hang problems.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
>>>> This doesn't look right:
>>>>
>>>> void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>>>> {
>>>>         if (err && cmd->retries) {
>>>>                 host->ops->request(host, mrq);
>>>>
>> This is NOT how it looks at mmc-next. You need to test with Adrian  
>> Hunters patch which the commit refers two.
> 
> In that case, how can I take the patch to mmci if it depends on something
> in another tree?
> 

I don't know. But how do you update your tree from next normally? I 
believe the problem is more related to that the mmc-next tree is now on 
a temporary git. If you do not update your tree how shall we be able to 
continue with integration of new patches that depends on mmc patches 
from "next"?

BR
Uffe

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Russell King - ARM Linux]

On Fri, Oct 14, 2011 at 09:51:44AM +0200, Ulf Hansson wrote:
> Russell King - ARM Linux wrote:
>> In that case, how can I take the patch to mmci if it depends on something
>> in another tree?
>>
>
> I don't know. But how do you update your tree from next normally?

I don't - no one in their right mind does.  linux-next is not a tree
which should ever be pulled into any other git tree to base work upon
- it's completely unstable and you can't rely on any commit in there
remaining.  The merge commits between each tree are also specific to
linux-next only.

Linus will also refuse to pull any tree which has linux-next merged
into it.

> I believe the problem is more related to that the mmc-next tree is now on  
> a temporary git.

Where a tree is hosted has no influence on whether its 'temporary' or
not.

> If you do not update your tree how shall we be able to  
> continue with integration of new patches that depends on mmc patches  
> from "next"?

That's what I'm saying - I can't take the patch as it introduces bugs
if I do.  It's better to either wait until after the next merge window,
or we have to sort out merging it via the mmc tree instead.

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Ulf Hansson]

>> If you do not update your tree how shall we be able to  
>> continue with integration of new patches that depends on mmc patches  
>> from "next"?
> 
> That's what I'm saying - I can't take the patch as it introduces bugs
> if I do.  It's better to either wait until after the next merge window,
> or we have to sort out merging it via the mmc tree instead.
> 

Thanks, now I really get the problem! :-)

 From mmci host driver perspective this will then be kind of complicated 
since it sometimes will depend on the patches on the mmc "framework".

Now we have the spinlock patch, but I believe this will happen soon 
again. So how can we handle to merge it via mmc tree instead? Will you 
be able to "Ack" patches for mmci so Chris Ball can merge them for the 
mmc tree instead? Of course I only mean patches that really has a 
dependency, the rest can be handled as is I believe.

BR
Uffe

[PATCH] mmc: mmci: Do not release spinlock in request_end

[Linus Walleij]

2011/10/14 Ulf Hansson <ulf.hansson@stericsson.com>:

> So how can we handle to merge it via mmc tree instead? Will you be able to
> "Ack" patches for mmci so Chris Ball can merge them for the mmc tree
> instead?

If you get Russell's ACK on it (provided it is merged on top of
Adrians patch) Chris can surely merge it into his tree.
He's already merging subsystem-wide patches (like Adrians)
to his tree, often touching several drivers in the process, as is
custom.

Have we checked if there are more spin_lock():s in the
other MMC host drivers that also should be removed as a
result of this or is MMCI the sole sinner?

Cross-tree dependencies is a mess, sadly we have no way
of solving that which is perfect. (Yet.)

Yours,
Linus Walleij