[PATCH] mmc: omap_hsmmc: Fix conditional locking
Thomas Gleixner
is no reason to keep the host->irq_lock across the call to
mmc_request_done(). Also the host->protect_card magic hack does not
depend on the context
Fix the mess by dropping host->irq_lock before calling
mmc_request_done().
Signed-off-by: Thomas Gleixner <tg...@linutronix.de>
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 4b23225..99a3383 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -468,14 +468,6 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
if (host->use_dma)
cmdreg |= DMA_EN;
- /*
- * In an interrupt context (i.e. STOP command), the spinlock is unlocked
- * by the interrupt handler, otherwise (i.e. for a new request) it is
- * unlocked here.
- */
- if (!in_interrupt())
- spin_unlock_irqrestore(&host->irq_lock, host->flags);
-
OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
}
@@ -506,7 +498,9 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
}
host->mrq = NULL;
+ spin_unlock(&host->irq_lock);
mmc_request_done(host->mmc, mrq);
+ spin_lock(&host->irq_lock);
return;
}
@@ -523,7 +517,9 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
if (!data->stop) {
host->mrq = NULL;
+ spin_unlock(&host->irq_lock);
mmc_request_done(host->mmc, data->mrq);
+ spin_lock(&host->irq_lock);
return;
}
omap_hsmmc_start_command(host, data->stop, NULL);
@@ -551,7 +547,9 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct mmc_command *cmd)
}
if ((host->data == NULL && !host->response_busy) || cmd->error) {
host->mrq = NULL;
+ spin_unlock(&host->irq_lock);
mmc_request_done(host->mmc, cmd->mrq);
+ spin_lock(&host->irq_lock);
}
}
@@ -1077,37 +1075,31 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
struct omap_hsmmc_host *host = mmc_priv(mmc);
int err;
+ spin_lock_irqsave(&host->irq_lock, host->flags);
/*
- * Prevent races with the interrupt handler because of unexpected
- * interrupts, but not if we are already in interrupt context i.e.
- * retries.
+ * Protect the card from I/O if there is a possibility
+ * it can be removed.
*/
- if (!in_interrupt()) {
- spin_lock_irqsave(&host->irq_lock, host->flags);
- /*
- * Protect the card from I/O if there is a possibility
- * it can be removed.
- */
- if (host->protect_card) {
- if (host->reqs_blocked < 3) {
- /*
- * Ensure the controller is left in a consistent
- * state by resetting the command and data state
- * machines.
- */
- omap_hsmmc_reset_controller_fsm(host, SRD);
- omap_hsmmc_reset_controller_fsm(host, SRC);
- host->reqs_blocked += 1;
- }
- req->cmd->error = -EBADF;
- if (req->data)
- req->data->error = -EBADF;
- spin_unlock_irqrestore(&host->irq_lock, host->flags);
- mmc_request_done(mmc, req);
- return;
- } else if (host->reqs_blocked)
- host->reqs_blocked = 0;
- }
+ if (host->protect_card) {
+ if (host->reqs_blocked < 3) {
+ /*
+ * Ensure the controller is left in a consistent
+ * state by resetting the command and data state
+ * machines.
+ */
+ omap_hsmmc_reset_controller_fsm(host, SRD);
+ omap_hsmmc_reset_controller_fsm(host, SRC);
+ host->reqs_blocked += 1;
+ }
+ req->cmd->error = -EBADF;
+ if (req->data)
+ req->data->error = -EBADF;
+ spin_unlock_irqrestore(&host->irq_lock, host->flags);
+ mmc_request_done(mmc, req);
+ return;
+ } else if (host->reqs_blocked)
+ host->reqs_blocked = 0;
+
WARN_ON(host->mrq != NULL);
host->mrq = req;
err = omap_hsmmc_prepare_data(host, req);
@@ -1116,13 +1108,13 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
if (req->data)
req->data->error = err;
host->mrq = NULL;
- if (!in_interrupt())
- spin_unlock_irqrestore(&host->irq_lock, host->flags);
+ spin_unlock_irqrestore(&host->irq_lock, host->flags);
mmc_request_done(mmc, req);
return;
}
omap_hsmmc_start_command(host, req->cmd, req->data);
+ spin_unlock_irqrestore(&host->irq_lock, host->flags);
}
/* Routine to configure clock values. Exposed API to core */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Madhusudhan
> -----Original Message-----
> From: linux-m...@vger.kernel.org [mailto:linux-mmc-
> ow...@vger.kernel.org] On Behalf Of Thomas Gleixner
> Sent: Monday, March 01, 2010 1:02 PM
> To: LKML
> Cc: linux...@vger.kernel.org; linu...@vger.kernel.org
> Subject: [PATCH] mmc: omap_hsmmc: Fix conditional locking
>
> Conditional locking on (!in_interrupt()) is broken by design and there
> is no reason to keep the host->irq_lock across the call to
> mmc_request_done(). Also the host->protect_card magic hack does not
> depend on the context
>
Can you please elaborate why the existing logic is broken?
It locks at the new request and unlocks just before issuing the cmd. Further
IRQ handler has these calls hence the !in_interrupt check.
How does this patch improve that? In fact with your patch for a data
transfer cmd there are several lock-unlock calls.
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
Thomas Gleixner
> > Conditional locking on (!in_interrupt()) is broken by design and there
> > is no reason to keep the host->irq_lock across the call to
> > mmc_request_done(). Also the host->protect_card magic hack does not
> > depend on the context
> >
>
> Can you please elaborate why the existing logic is broken?
Locks are only to be held to serialize data or state.
The mmc_request_done() call does _NOT_ require that at all. So
dropping the lock there is the right thing to do.
Also conditional locking on in_interrupt() is generally a nono as it
relies on assumptions which are not necessarily true in all
circumstances. Just one simple example: interrupt threading will make
it explode nicely and it did already with the realtime patches
applied.
Such code constructs prevent us to do generic changes to the kernel
behaviour without any real good reason.
> It locks at the new request and unlocks just before issuing the cmd. Further
> IRQ handler has these calls hence the !in_interrupt check.
Aside of the conditional locking I have several issues with that code:
1) The code flow is massively unreadable:
omap_hsmmc_start_command()
{
.....
if (!in_interrupt())
spin_unlock_irq();
}
omap_hsmmc_request()
{
if (!in_interrupt())
spin_lock_irq();
omap_hsmmc_start_command();
}
We generally want to see the lock/unlock pairs in one function and not
having to figure out where the heck unlock happens.
2) The point of unlocking is patently wrong
omap_hsmmc_start_command()
{
.....
if (!in_interrupt())
spin_unlock_irq();
--->
OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
--->
OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
}
What happens, if you get a spurious interrupt here ? Same for SMP,
though you are probably protected by the core mmc code request
serialization there.
> How does this patch improve that? In fact with your patch for a data
> transfer cmd there are several lock-unlock calls.
1) The patch simply removes conditional locking and moves the lock
sections to those places which protect something. Aside of that it
makes the code easier to understand.
2) What's the point of not having those lock/unlocks ? On UP the
spinlock is a NOOP anyway, so you won't even notice. On SMP you
won't notice either, simply because the lock is cache hot and
almost never contended.
Thanks,
tglx
Madhusudhan
Sounds reasonable.Readbility is still a factor but works for me as the main
intention here is to remove the in_interrupt conditional.
Acked-by: Madhusudhan Chikkature<madh...@ti.com>
Best Regards,
Madhu
Adrian Hunter
> Conditional locking on (!in_interrupt()) is broken by design and there
> is no reason to keep the host->irq_lock across the call to
> mmc_request_done(). Also the host->protect_card magic hack does not
> depend on the context
Card protect does depend on not being invoked in the middle of a request.
For example, it could result in a open-ended read being started but never
stopped.
in_interrupt() can be replaced with a new flag 'in_request' that is set
in omap_hsmmc_request() and cleared after mmc_request_done()
Also, as you allude to in another email, the spinlock, while preventing an
oops, does not stop the unwanted interrupt, which will cause problems.
I can make the necessary changes but it will be a few days before
I have the time.
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in