* [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-06-20 16:27 ` Guennadi Liakhovetski
0 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-06-20 16:27 UTC (permalink / raw)
To: linux-mmc
Cc: linux-sh, Ian Molton, Kuninori Morimoto, Chris Ball, Magnus Damm
Calling mmc_request_done() under a spinlock with interrupts disabled
leads to a recursive spin-lock on request retry path and to
scheduling in atomic context. This patch fixes both these problems
by moving mmc_request_done() to the scheduler workqueue.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
so, reviews and tests are highly appreciated! Also, unfortunately, I
wasn't able to test it well enough with SDIO, because the driver for the
only SDIO card, that I have, reproducibly crashes the kernel:
http://www.spinics.net/lists/kernel/msg1203334.html
So, mainly tested with SD-cards and only lightly with SDIO.
v2:
1. added a mutex to properly complete each .set_ios() call instead of
returning an error, when racing with another one.
2. oritect data inside tmio_mmc_finish_request()
3. don't reschedule card detection, if one is already pending
drivers/mmc/host/tmio_mmc.h | 6 +++++-
drivers/mmc/host/tmio_mmc_pio.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 8260bc2..5b83e46 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -18,6 +18,7 @@
#include <linux/highmem.h>
#include <linux/mmc/tmio.h>
+#include <linux/mutex.h>
#include <linux/pagemap.h>
#include <linux/spinlock.h>
@@ -73,8 +74,11 @@ struct tmio_mmc_host {
/* Track lost interrupts */
struct delayed_work delayed_reset_work;
- spinlock_t lock;
+ struct work_struct done;
+
+ spinlock_t lock; /* protect host private data */
unsigned long last_req_ts;
+ struct mutex ios_lock; /* protect set_ios() context */
};
int tmio_mmc_host_probe(struct tmio_mmc_host **host,
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 0b09e82..eabda39 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -284,10 +284,16 @@ static void tmio_mmc_reset_work(struct work_struct *work)
/* called with host->lock held, interrupts disabled */
static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
{
- struct mmc_request *mrq = host->mrq;
+ struct mmc_request *mrq;
+ unsigned long flags;
- if (!mrq)
+ spin_lock_irqsave(&host->lock, flags);
+
+ mrq = host->mrq;
+ if (IS_ERR_OR_NULL(mrq)) {
+ spin_unlock_irqrestore(&host->lock, flags);
return;
+ }
host->cmd = NULL;
host->data = NULL;
@@ -296,11 +302,18 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
cancel_delayed_work(&host->delayed_reset_work);
host->mrq = NULL;
+ spin_unlock_irqrestore(&host->lock, flags);
- /* FIXME: mmc_request_done() can schedule! */
mmc_request_done(host->mmc, mrq);
}
+static void tmio_mmc_done_work(struct work_struct *work)
+{
+ struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host,
+ done);
+ tmio_mmc_finish_request(host);
+}
+
/* These are the bitmasks the tmio chip requires to implement the MMC response
* types. Note that R1 and R6 are the same in this scheme. */
#define APP_CMD 0x0040
@@ -467,7 +480,7 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
BUG();
}
- tmio_mmc_finish_request(host);
+ schedule_work(&host->done);
}
static void tmio_mmc_data_irq(struct tmio_mmc_host *host)
@@ -557,7 +570,7 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host,
tasklet_schedule(&host->dma_issue);
}
} else {
- tmio_mmc_finish_request(host);
+ schedule_work(&host->done);
}
out:
@@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
TMIO_STAT_CARD_REMOVE);
- mmc_detect_change(host->mmc, msecs_to_jiffies(100));
+ if (!work_pending(&host->mmc->detect.work))
+ mmc_detect_change(host->mmc, msecs_to_jiffies(100));
}
/* CRC and other errors */
@@ -749,6 +763,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
struct tmio_mmc_data *pdata = host->pdata;
unsigned long flags;
+ mutex_lock(&host->ios_lock);
+
spin_lock_irqsave(&host->lock, flags);
if (host->mrq) {
if (IS_ERR(host->mrq)) {
@@ -764,6 +780,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
host->mrq->cmd->opcode, host->last_req_ts, jiffies);
}
spin_unlock_irqrestore(&host->lock, flags);
+
+ mutex_unlock(&host->ios_lock);
return;
}
@@ -817,6 +835,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
current->comm, task_pid_nr(current),
ios->clock, ios->power_mode);
host->mrq = NULL;
+
+ mutex_unlock(&host->ios_lock);
}
static int tmio_mmc_get_ro(struct mmc_host *mmc)
@@ -913,9 +933,11 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
tmio_mmc_enable_sdio_irq(mmc, 0);
spin_lock_init(&_host->lock);
+ mutex_init(&_host->ios_lock);
/* Init delayed work for request timeouts */
INIT_DELAYED_WORK(&_host->delayed_reset_work, tmio_mmc_reset_work);
+ INIT_WORK(&_host->done, tmio_mmc_done_work);
/* See if we also get DMA */
tmio_mmc_request_dma(_host, pdata);
@@ -963,6 +985,7 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host)
pm_runtime_get_sync(&pdev->dev);
mmc_remove_host(host->mmc);
+ cancel_work_sync(&host->done);
cancel_delayed_work_sync(&host->delayed_reset_work);
tmio_mmc_release_dma(host);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
2011-06-20 16:27 ` [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled Guennadi Liakhovetski
@ 2011-07-12 10:51 ` kuninori.morimoto.gx
-1 siblings, 0 replies; 18+ messages in thread
From: kuninori.morimoto.gx @ 2011-07-12 10:51 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc, linux-sh, Ian Molton, Chris Ball, Magnus Damm
> Calling mmc_request_done() under a spinlock with interrupts disabled
> leads to a recursive spin-lock on request retry path and to
> scheduling in atomic context. This patch fixes both these problems
> by moving mmc_request_done() to the scheduler workqueue.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
I tested this patch, and it solved boot hungup issue on Ecovec board.
Thanks Guennadi. (and sorry for the extended delay)
Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-07-12 10:51 ` kuninori.morimoto.gx
0 siblings, 0 replies; 18+ messages in thread
From: kuninori.morimoto.gx @ 2011-07-12 10:51 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc, linux-sh, Ian Molton, Chris Ball, Magnus Damm
> Calling mmc_request_done() under a spinlock with interrupts disabled
> leads to a recursive spin-lock on request retry path and to
> scheduling in atomic context. This patch fixes both these problems
> by moving mmc_request_done() to the scheduler workqueue.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
I tested this patch, and it solved boot hungup issue on Ecovec board.
Thanks Guennadi. (and sorry for the extended delay)
Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
2011-06-20 16:27 ` [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled Guennadi Liakhovetski
@ 2011-07-13 15:00 ` Chris Ball
-1 siblings, 0 replies; 18+ messages in thread
From: Chris Ball @ 2011-07-13 15:00 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Guennadi,
On Mon, Jun 20 2011, Guennadi Liakhovetski wrote:
> Calling mmc_request_done() under a spinlock with interrupts disabled
> leads to a recursive spin-lock on request retry path and to
> scheduling in atomic context. This patch fixes both these problems
> by moving mmc_request_done() to the scheduler workqueue.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
> it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
> so, reviews and tests are highly appreciated! Also, unfortunately, I
> wasn't able to test it well enough with SDIO, because the driver for the
> only SDIO card, that I have, reproducibly crashes the kernel:
Having trouble working out how to apply this -- for example, in this hunk:
> @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
> TMIO_STAT_CARD_REMOVE);
> - mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> + if (!work_pending(&host->mmc->detect.work))
> + mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> }
>
> /* CRC and other errors */
In mmc-next there's a "goto out;" after the mmc_detect_change call, which
looks like it's always been there. Am I missing a patch this depends on?
(It'd be a good time to get a full set of tmio patches for 3.1 pulled
together, if you can do that.)
Thanks!
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-07-13 15:00 ` Chris Ball
0 siblings, 0 replies; 18+ messages in thread
From: Chris Ball @ 2011-07-13 15:00 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Guennadi,
On Mon, Jun 20 2011, Guennadi Liakhovetski wrote:
> Calling mmc_request_done() under a spinlock with interrupts disabled
> leads to a recursive spin-lock on request retry path and to
> scheduling in atomic context. This patch fixes both these problems
> by moving mmc_request_done() to the scheduler workqueue.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
> it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
> so, reviews and tests are highly appreciated! Also, unfortunately, I
> wasn't able to test it well enough with SDIO, because the driver for the
> only SDIO card, that I have, reproducibly crashes the kernel:
Having trouble working out how to apply this -- for example, in this hunk:
> @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
> TMIO_STAT_CARD_REMOVE);
> - mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> + if (!work_pending(&host->mmc->detect.work))
> + mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> }
>
> /* CRC and other errors */
In mmc-next there's a "goto out;" after the mmc_detect_change call, which
looks like it's always been there. Am I missing a patch this depends on?
(It'd be a good time to get a full set of tmio patches for 3.1 pulled
together, if you can do that.)
Thanks!
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule
2011-07-13 15:00 ` Chris Ball
@ 2011-07-13 23:33 ` Guennadi Liakhovetski
-1 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-07-13 23:33 UTC (permalink / raw)
To: Chris Ball
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Chris
On Wed, 13 Jul 2011, Chris Ball wrote:
> Hi Guennadi,
>
> On Mon, Jun 20 2011, Guennadi Liakhovetski wrote:
> > Calling mmc_request_done() under a spinlock with interrupts disabled
> > leads to a recursive spin-lock on request retry path and to
> > scheduling in atomic context. This patch fixes both these problems
> > by moving mmc_request_done() to the scheduler workqueue.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
> > it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
> > so, reviews and tests are highly appreciated! Also, unfortunately, I
> > wasn't able to test it well enough with SDIO, because the driver for the
> > only SDIO card, that I have, reproducibly crashes the kernel:
>
> Having trouble working out how to apply this -- for example, in this hunk:
>
> > @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> > tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
> > TMIO_STAT_CARD_REMOVE);
> > - mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > + if (!work_pending(&host->mmc->detect.work))
> > + mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > }
> >
> > /* CRC and other errors */
>
> In mmc-next there's a "goto out;" after the mmc_detect_change call, which
> looks like it's always been there. Am I missing a patch this depends on?
>
> (It'd be a good time to get a full set of tmio patches for 3.1 pulled
> together, if you can do that.)
Ok, if you don't mind, I'll get a couple of hours of sleep, and will reply
to you first thing in the morning:-)
Thanks
Guennadi
>
> Thanks!
>
> - Chris.
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-07-13 23:33 ` Guennadi Liakhovetski
0 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-07-13 23:33 UTC (permalink / raw)
To: Chris Ball
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Chris
On Wed, 13 Jul 2011, Chris Ball wrote:
> Hi Guennadi,
>
> On Mon, Jun 20 2011, Guennadi Liakhovetski wrote:
> > Calling mmc_request_done() under a spinlock with interrupts disabled
> > leads to a recursive spin-lock on request retry path and to
> > scheduling in atomic context. This patch fixes both these problems
> > by moving mmc_request_done() to the scheduler workqueue.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
> > it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
> > so, reviews and tests are highly appreciated! Also, unfortunately, I
> > wasn't able to test it well enough with SDIO, because the driver for the
> > only SDIO card, that I have, reproducibly crashes the kernel:
>
> Having trouble working out how to apply this -- for example, in this hunk:
>
> > @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> > tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
> > TMIO_STAT_CARD_REMOVE);
> > - mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > + if (!work_pending(&host->mmc->detect.work))
> > + mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > }
> >
> > /* CRC and other errors */
>
> In mmc-next there's a "goto out;" after the mmc_detect_change call, which
> looks like it's always been there. Am I missing a patch this depends on?
>
> (It'd be a good time to get a full set of tmio patches for 3.1 pulled
> together, if you can do that.)
Ok, if you don't mind, I'll get a couple of hours of sleep, and will reply
to you first thing in the morning:-)
Thanks
Guennadi
>
> Thanks!
>
> - Chris.
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
2011-07-13 23:33 ` [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled Guennadi Liakhovetski
@ 2011-07-14 0:39 ` Chris Ball
-1 siblings, 0 replies; 18+ messages in thread
From: Chris Ball @ 2011-07-14 0:39 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Guennadi,
On Wed, Jul 13 2011, Guennadi Liakhovetski wrote:
>> In mmc-next there's a "goto out;" after the mmc_detect_change call, which
>> looks like it's always been there. Am I missing a patch this depends on?
>>
>> (It'd be a good time to get a full set of tmio patches for 3.1 pulled
>> together, if you can do that.)
>
> Ok, if you don't mind, I'll get a couple of hours of sleep, and will reply
> to you first thing in the morning:-)
Not only do I not mind, I'd strongly prefer if you did. ;-)
Thanks very much,
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-07-14 0:39 ` Chris Ball
0 siblings, 0 replies; 18+ messages in thread
From: Chris Ball @ 2011-07-14 0:39 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Guennadi,
On Wed, Jul 13 2011, Guennadi Liakhovetski wrote:
>> In mmc-next there's a "goto out;" after the mmc_detect_change call, which
>> looks like it's always been there. Am I missing a patch this depends on?
>>
>> (It'd be a good time to get a full set of tmio patches for 3.1 pulled
>> together, if you can do that.)
>
> Ok, if you don't mind, I'll get a couple of hours of sleep, and will reply
> to you first thing in the morning:-)
Not only do I not mind, I'd strongly prefer if you did. ;-)
Thanks very much,
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule
2011-07-13 15:00 ` Chris Ball
@ 2011-07-14 10:26 ` Guennadi Liakhovetski
-1 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-07-14 10:26 UTC (permalink / raw)
To: Chris Ball
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Chris
On Wed, 13 Jul 2011, Chris Ball wrote:
> Hi Guennadi,
>
> On Mon, Jun 20 2011, Guennadi Liakhovetski wrote:
> > Calling mmc_request_done() under a spinlock with interrupts disabled
> > leads to a recursive spin-lock on request retry path and to
> > scheduling in atomic context. This patch fixes both these problems
> > by moving mmc_request_done() to the scheduler workqueue.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
> > it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
> > so, reviews and tests are highly appreciated! Also, unfortunately, I
> > wasn't able to test it well enough with SDIO, because the driver for the
> > only SDIO card, that I have, reproducibly crashes the kernel:
>
> Having trouble working out how to apply this -- for example, in this hunk:
Right, I've been developing on top of other trees. As you probably have
seen, a couple of minutes ago I've updated my two patches and re-posted
them:
[PATCH v3] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
[PATCH v3] mmc: tmio: maximize power saving
That's it for this merge window for tmio from me so far;-) The outstanding
sh-mmcif patch
[PATCH/RFC] mmc: sh_mmcif: maximize power saving
is no longer an RFC, should have no conflicts and requires no updates.
Thanks
Guennadi
>
> > @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> > tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
> > TMIO_STAT_CARD_REMOVE);
> > - mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > + if (!work_pending(&host->mmc->detect.work))
> > + mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > }
> >
> > /* CRC and other errors */
>
> In mmc-next there's a "goto out;" after the mmc_detect_change call, which
> looks like it's always been there. Am I missing a patch this depends on?
>
> (It'd be a good time to get a full set of tmio patches for 3.1 pulled
> together, if you can do that.)
>
> Thanks!
>
> - Chris.
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-07-14 10:26 ` Guennadi Liakhovetski
0 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-07-14 10:26 UTC (permalink / raw)
To: Chris Ball
Cc: linux-mmc, linux-sh, Ian Molton, Kuninori Morimoto, Magnus Damm
Hi Chris
On Wed, 13 Jul 2011, Chris Ball wrote:
> Hi Guennadi,
>
> On Mon, Jun 20 2011, Guennadi Liakhovetski wrote:
> > Calling mmc_request_done() under a spinlock with interrupts disabled
> > leads to a recursive spin-lock on request retry path and to
> > scheduling in atomic context. This patch fixes both these problems
> > by moving mmc_request_done() to the scheduler workqueue.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
> > it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
> > so, reviews and tests are highly appreciated! Also, unfortunately, I
> > wasn't able to test it well enough with SDIO, because the driver for the
> > only SDIO card, that I have, reproducibly crashes the kernel:
>
> Having trouble working out how to apply this -- for example, in this hunk:
Right, I've been developing on top of other trees. As you probably have
seen, a couple of minutes ago I've updated my two patches and re-posted
them:
[PATCH v3] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
[PATCH v3] mmc: tmio: maximize power saving
That's it for this merge window for tmio from me so far;-) The outstanding
sh-mmcif patch
[PATCH/RFC] mmc: sh_mmcif: maximize power saving
is no longer an RFC, should have no conflicts and requires no updates.
Thanks
Guennadi
>
> > @@ -618,7 +631,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
> > tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
> > TMIO_STAT_CARD_REMOVE);
> > - mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > + if (!work_pending(&host->mmc->detect.work))
> > + mmc_detect_change(host->mmc, msecs_to_jiffies(100));
> > }
> >
> > /* CRC and other errors */
>
> In mmc-next there's a "goto out;" after the mmc_detect_change call, which
> looks like it's always been there. Am I missing a patch this depends on?
>
> (It'd be a good time to get a full set of tmio patches for 3.1 pulled
> together, if you can do that.)
>
> Thanks!
>
> - Chris.
> --
> Chris Ball <cjb@laptop.org> <http://printf.net/>
> One Laptop Per Child
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] mmc: tmio: fix recursive spinlock, don't schedule with
2011-06-20 16:27 ` [PATCH v2] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled Guennadi Liakhovetski
@ 2011-07-14 10:12 ` Guennadi Liakhovetski
-1 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-07-14 10:12 UTC (permalink / raw)
To: linux-mmc
Cc: linux-sh, Ian Molton, Kuninori Morimoto, Chris Ball, Magnus Damm
Calling mmc_request_done() under a spinlock with interrupts disabled
leads to a recursive spin-lock on request retry path and to
scheduling in atomic context. This patch fixes both these problems
by moving mmc_request_done() to the scheduler workqueue.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
so, reviews and tests are highly appreciated! Also, unfortunately, I
wasn't able to test it well enough with SDIO, because the driver for the
only SDIO card, that I have, reproducibly crashes the kernel:
http://www.spinics.net/lists/kernel/msg1203334.html
So, mainly tested with SD-cards and only lightly with SDIO.
v3: no functional changes, only updated on top of current mmc-next
v2:
1. added a mutex to properly complete each .set_ios() call instead of
returning an error, when racing with another one.
2. oritect data inside tmio_mmc_finish_request()
3. don't reschedule card detection, if one is already pending
drivers/mmc/host/tmio_mmc.h | 6 +++++-
drivers/mmc/host/tmio_mmc_pio.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 0c22df0..a2d6f9f 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -18,6 +18,7 @@
#include <linux/highmem.h>
#include <linux/mmc/tmio.h>
+#include <linux/mutex.h>
#include <linux/pagemap.h>
#include <linux/spinlock.h>
@@ -73,8 +74,11 @@ struct tmio_mmc_host {
/* Track lost interrupts */
struct delayed_work delayed_reset_work;
- spinlock_t lock;
+ struct work_struct done;
+
+ spinlock_t lock; /* protect host private data */
unsigned long last_req_ts;
+ struct mutex ios_lock; /* protect set_ios() context */
};
int tmio_mmc_host_probe(struct tmio_mmc_host **host,
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index f7dd3b1..a2f76ad 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -250,10 +250,16 @@ static void tmio_mmc_reset_work(struct work_struct *work)
/* called with host->lock held, interrupts disabled */
static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
{
- struct mmc_request *mrq = host->mrq;
+ struct mmc_request *mrq;
+ unsigned long flags;
- if (!mrq)
+ spin_lock_irqsave(&host->lock, flags);
+
+ mrq = host->mrq;
+ if (IS_ERR_OR_NULL(mrq)) {
+ spin_unlock_irqrestore(&host->lock, flags);
return;
+ }
host->cmd = NULL;
host->data = NULL;
@@ -262,11 +268,18 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
cancel_delayed_work(&host->delayed_reset_work);
host->mrq = NULL;
+ spin_unlock_irqrestore(&host->lock, flags);
- /* FIXME: mmc_request_done() can schedule! */
mmc_request_done(host->mmc, mrq);
}
+static void tmio_mmc_done_work(struct work_struct *work)
+{
+ struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host,
+ done);
+ tmio_mmc_finish_request(host);
+}
+
/* These are the bitmasks the tmio chip requires to implement the MMC response
* types. Note that R1 and R6 are the same in this scheme. */
#define APP_CMD 0x0040
@@ -433,7 +446,7 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
BUG();
}
- tmio_mmc_finish_request(host);
+ schedule_work(&host->done);
}
static void tmio_mmc_data_irq(struct tmio_mmc_host *host)
@@ -523,7 +536,7 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host,
tasklet_schedule(&host->dma_issue);
}
} else {
- tmio_mmc_finish_request(host);
+ schedule_work(&host->done);
}
out:
@@ -573,7 +586,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
TMIO_STAT_CARD_REMOVE);
- mmc_detect_change(host->mmc, msecs_to_jiffies(100));
+ if (!work_pending(&host->mmc->detect.work))
+ mmc_detect_change(host->mmc, msecs_to_jiffies(100));
goto out;
}
@@ -703,6 +717,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
struct tmio_mmc_data *pdata = host->pdata;
unsigned long flags;
+ mutex_lock(&host->ios_lock);
+
spin_lock_irqsave(&host->lock, flags);
if (host->mrq) {
if (IS_ERR(host->mrq)) {
@@ -718,6 +734,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
host->mrq->cmd->opcode, host->last_req_ts, jiffies);
}
spin_unlock_irqrestore(&host->lock, flags);
+
+ mutex_unlock(&host->ios_lock);
return;
}
@@ -771,6 +789,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
current->comm, task_pid_nr(current),
ios->clock, ios->power_mode);
host->mrq = NULL;
+
+ mutex_unlock(&host->ios_lock);
}
static int tmio_mmc_get_ro(struct mmc_host *mmc)
@@ -867,9 +887,11 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
tmio_mmc_enable_sdio_irq(mmc, 0);
spin_lock_init(&_host->lock);
+ mutex_init(&_host->ios_lock);
/* Init delayed work for request timeouts */
INIT_DELAYED_WORK(&_host->delayed_reset_work, tmio_mmc_reset_work);
+ INIT_WORK(&_host->done, tmio_mmc_done_work);
/* See if we also get DMA */
tmio_mmc_request_dma(_host, pdata);
@@ -917,6 +939,7 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host)
pm_runtime_get_sync(&pdev->dev);
mmc_remove_host(host->mmc);
+ cancel_work_sync(&host->done);
cancel_delayed_work_sync(&host->delayed_reset_work);
tmio_mmc_release_dma(host);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3] mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled
@ 2011-07-14 10:12 ` Guennadi Liakhovetski
0 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2011-07-14 10:12 UTC (permalink / raw)
To: linux-mmc
Cc: linux-sh, Ian Molton, Kuninori Morimoto, Chris Ball, Magnus Damm
Calling mmc_request_done() under a spinlock with interrupts disabled
leads to a recursive spin-lock on request retry path and to
scheduling in atomic context. This patch fixes both these problems
by moving mmc_request_done() to the scheduler workqueue.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
This is a bug-fix: without it the system Oopses with LOCKDEP enabled, so,
it should really go in 3.0. OTOH it is pretty intrusine and non-trivial,
so, reviews and tests are highly appreciated! Also, unfortunately, I
wasn't able to test it well enough with SDIO, because the driver for the
only SDIO card, that I have, reproducibly crashes the kernel:
http://www.spinics.net/lists/kernel/msg1203334.html
So, mainly tested with SD-cards and only lightly with SDIO.
v3: no functional changes, only updated on top of current mmc-next
v2:
1. added a mutex to properly complete each .set_ios() call instead of
returning an error, when racing with another one.
2. oritect data inside tmio_mmc_finish_request()
3. don't reschedule card detection, if one is already pending
drivers/mmc/host/tmio_mmc.h | 6 +++++-
drivers/mmc/host/tmio_mmc_pio.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 0c22df0..a2d6f9f 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -18,6 +18,7 @@
#include <linux/highmem.h>
#include <linux/mmc/tmio.h>
+#include <linux/mutex.h>
#include <linux/pagemap.h>
#include <linux/spinlock.h>
@@ -73,8 +74,11 @@ struct tmio_mmc_host {
/* Track lost interrupts */
struct delayed_work delayed_reset_work;
- spinlock_t lock;
+ struct work_struct done;
+
+ spinlock_t lock; /* protect host private data */
unsigned long last_req_ts;
+ struct mutex ios_lock; /* protect set_ios() context */
};
int tmio_mmc_host_probe(struct tmio_mmc_host **host,
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index f7dd3b1..a2f76ad 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -250,10 +250,16 @@ static void tmio_mmc_reset_work(struct work_struct *work)
/* called with host->lock held, interrupts disabled */
static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
{
- struct mmc_request *mrq = host->mrq;
+ struct mmc_request *mrq;
+ unsigned long flags;
- if (!mrq)
+ spin_lock_irqsave(&host->lock, flags);
+
+ mrq = host->mrq;
+ if (IS_ERR_OR_NULL(mrq)) {
+ spin_unlock_irqrestore(&host->lock, flags);
return;
+ }
host->cmd = NULL;
host->data = NULL;
@@ -262,11 +268,18 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
cancel_delayed_work(&host->delayed_reset_work);
host->mrq = NULL;
+ spin_unlock_irqrestore(&host->lock, flags);
- /* FIXME: mmc_request_done() can schedule! */
mmc_request_done(host->mmc, mrq);
}
+static void tmio_mmc_done_work(struct work_struct *work)
+{
+ struct tmio_mmc_host *host = container_of(work, struct tmio_mmc_host,
+ done);
+ tmio_mmc_finish_request(host);
+}
+
/* These are the bitmasks the tmio chip requires to implement the MMC response
* types. Note that R1 and R6 are the same in this scheme. */
#define APP_CMD 0x0040
@@ -433,7 +446,7 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host)
BUG();
}
- tmio_mmc_finish_request(host);
+ schedule_work(&host->done);
}
static void tmio_mmc_data_irq(struct tmio_mmc_host *host)
@@ -523,7 +536,7 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host,
tasklet_schedule(&host->dma_issue);
}
} else {
- tmio_mmc_finish_request(host);
+ schedule_work(&host->done);
}
out:
@@ -573,7 +586,8 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
TMIO_STAT_CARD_REMOVE);
- mmc_detect_change(host->mmc, msecs_to_jiffies(100));
+ if (!work_pending(&host->mmc->detect.work))
+ mmc_detect_change(host->mmc, msecs_to_jiffies(100));
goto out;
}
@@ -703,6 +717,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
struct tmio_mmc_data *pdata = host->pdata;
unsigned long flags;
+ mutex_lock(&host->ios_lock);
+
spin_lock_irqsave(&host->lock, flags);
if (host->mrq) {
if (IS_ERR(host->mrq)) {
@@ -718,6 +734,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
host->mrq->cmd->opcode, host->last_req_ts, jiffies);
}
spin_unlock_irqrestore(&host->lock, flags);
+
+ mutex_unlock(&host->ios_lock);
return;
}
@@ -771,6 +789,8 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
current->comm, task_pid_nr(current),
ios->clock, ios->power_mode);
host->mrq = NULL;
+
+ mutex_unlock(&host->ios_lock);
}
static int tmio_mmc_get_ro(struct mmc_host *mmc)
@@ -867,9 +887,11 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
tmio_mmc_enable_sdio_irq(mmc, 0);
spin_lock_init(&_host->lock);
+ mutex_init(&_host->ios_lock);
/* Init delayed work for request timeouts */
INIT_DELAYED_WORK(&_host->delayed_reset_work, tmio_mmc_reset_work);
+ INIT_WORK(&_host->done, tmio_mmc_done_work);
/* See if we also get DMA */
tmio_mmc_request_dma(_host, pdata);
@@ -917,6 +939,7 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host)
pm_runtime_get_sync(&pdev->dev);
mmc_remove_host(host->mmc);
+ cancel_work_sync(&host->done);
cancel_delayed_work_sync(&host->delayed_reset_work);
tmio_mmc_release_dma(host);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 18+ messages in thread