mmc: mmci: Do not release spinlock in request_end
Subsystems (2)
PATCH 1/1 mmc: mmci: Do not release spinlock in request_end
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.
--- drivers/mmc/host/mmci.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
drivers/mmc/host/mmci.c +0 -6
@@ -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)
Signature · 1 line
1.7.5.4
PATCH 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 6 lines · 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 <[email protected]>
Adrian do you agree with this?
To me it looks correct,
Yours,
Linus Walleij
PATCH 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 10 lines · Linus Walleij
Quoted 6 lines · 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 <[email protected]>Adrian do you agree with this?
Yes
Quoted 6 lines
To me it looks correct,
Acked-by Linus Walleij <[email protected]>Yours,
Linus Walleij
PATCH 1/1 mmc: mmci: Do not release spinlock in request_end
In reply to Ulf HanssonQuoted 6 lines · 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 <[email protected]>
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 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 10 lines · Russell King - ARM Linux
Quoted 6 lines · 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 <[email protected]>This doesn't look right:
Quoted 8 lines
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.
Signature · 1 line
Tixy
PATCH 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 7 lines · Jon Medhurst (Tixy)
Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Signed-off-by Ulf Hansson <[email protected]>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
Quoted 7 lines
Quoted 3 lines
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 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 11 lines · Ulf Hansson
Quoted 10 lines · Jon Medhurst (Tixy)
Quoted 9 lines · Russell King - ARM Linux
Quoted 6 lines · 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 <[email protected]>This doesn't look right:
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
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 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 17 lines · Russell King - ARM Linux
Quoted 16 lines · Ulf Hansson
Quoted 15 lines · Jon Medhurst (Tixy)
Quoted 14 lines · Russell King - ARM Linux
Quoted 6 lines · 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 <[email protected]>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);
Quoted 6 lines
Quoted 2 lines
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 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 6 lines · Ulf Hansson
Quoted 3 lines · Russell King - ARM Linux
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.
Quoted 2 lines
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.
Quoted 3 lines
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 1/1 mmc: mmci: Do not release spinlock in request_end
Quoted 8 lines
Quoted 3 lines
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 1/1 mmc: mmci: Do not release spinlock in request_end
2011/10/14 Ulf Hansson <[email protected]>:
Quoted 3 lines
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