lkml.org 
[lkml]   [2010]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mmc: omap_hsmmc: Fix conditional locking
    Thomas Gleixner wrote:
    > 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.

    >
    > Fix the mess by dropping host->irq_lock before calling
    > mmc_request_done().
    >
    > Signed-off-by: Thomas Gleixner <tglx@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-omap" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    >
    >



    \
     
     \ /
      Last update: 2010-03-06 13:57    [W:0.076 / U:0.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site