From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755168Ab0CCBhx (ORCPT ); Tue, 2 Mar 2010 20:37:53 -0500 Received: from bear.ext.ti.com ([192.94.94.41]:57547 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752335Ab0CCBhv (ORCPT ); Tue, 2 Mar 2010 20:37:51 -0500 From: "Madhusudhan" To: "'Thomas Gleixner'" , "'LKML'" Cc: , References: Subject: RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking Date: Tue, 2 Mar 2010 19:37:42 -0600 Message-ID: <010101caba72$1dd1d790$544ff780@am.dhcp.ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Office Outlook 11 Thread-Index: Acq5ccm01hwLtk7mSZaMXb9C1DpuIwA/44nw In-Reply-To: X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.3198 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc- > owner@vger.kernel.org] On Behalf Of Thomas Gleixner > Sent: Monday, March 01, 2010 1:02 PM > To: LKML > Cc: linux-omap@vger.kernel.org; linux-mmc@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. > Fix the mess by dropping host->irq_lock before calling > mmc_request_done(). > > Signed-off-by: Thomas Gleixner > > 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-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html