Re: [PATCH 26/32] omap_hsmmc: prevent races with irq handler

[Adrian Hunter <adrian.hunter@nokia.com>]

Andrew Morton wrote:
> On Fri, 10 Jul 2009 15:43:09 +0300
> Adrian Hunter <adrian.hunter@nokia.com> wrote:
> 
>> >From 242fae6293adec671b14354f215217354f5076a0 Mon Sep 17 00:00:00 2001
>> From: Adrian Hunter <adrian.hunter@nokia.com>
>> Date: Sat, 16 May 2009 10:32:34 +0300
>> Subject: [PATCH] omap_hsmmc: prevent races with irq handler
>>
>> If an unexpected interrupt occurs while preparing the
>> next request, an oops can occur.
>>
>> For example, a new request is setting up DMA for data
>> transfer so host->data is not NULL.  An unexpected
>> transfer complete (TC) interrupt comes along and
>> the interrupt handler sets host->data to NULL.  Oops!
>>
>> Prevent that by disabling interrupts while setting up
>> a new request.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
>> ---
>>  drivers/mmc/host/omap_hsmmc.c |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 28563d6..38e1410 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -452,6 +452,13 @@ mmc_omap_start_command(struct mmc_omap_host *host, struct mmc_command *cmd,
>>  	if (host->use_dma)
>>  		cmdreg |= DMA_EN;
>>  
>> +	/*
>> +	 * In an interrupt context (i.e. STOP command), the interrupt is already
>> +	 * enabled, otherwise it is not (i.e. new request).
>> +	 */
>> +	if (!in_interrupt())
>> +		enable_irq(host->irq);
>> +
>>  	OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
>>  	OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
>>  }
>> @@ -1011,6 +1018,13 @@ static void omap_mmc_request(struct mmc_host *mmc, struct mmc_request *req)
>>  	struct mmc_omap_host *host = mmc_priv(mmc);
>>  	int err;
>>  
>> +	/*
>> +	 * Prevent races with the interrupt handler because of unexpected
>> +	 * interrupts, but not if we are already in interrupt context i.e.
>> +	 * retries.
>> +	 */
>> +	if (!in_interrupt())
>> +		disable_irq(host->irq);
>>  	WARN_ON(host->mrq != NULL);
>>  	host->mrq = req;
>>  	err = mmc_omap_prepare_data(host, req);
>> @@ -1019,6 +1033,8 @@ static void omap_mmc_request(struct mmc_host *mmc, struct mmc_request *req)
>>  		if (req->data)
>>  			req->data->error = err;
>>  		host->mrq = NULL;
>> +		if (!in_interrupt())
>> +			enable_irq(host->irq);
>>  		mmc_request_done(mmc, req);
>>  		return;
>>  	}
> 
> That seems pretty crude.  Disabling an interrupt line can be expensive,
> and will shut off any other innocent devices which share the line.
> 
> The usual and superior way of fixing races such as this is
> spin_lock_irq[save]().
> 
> 

>From f54be3678907b85a00d9f51e3ce4c828fc21ee95 Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@nokia.com>
Date: Mon, 27 Jul 2009 12:00:17 +0300
Subject: [PATCH] omap_hsmmc: use spin_lock not disable_irq

Disabling an interrupt line can be expensive.  The usual and
superior way of fixing races such as this is spin_lock_irqsave.

Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
---
 drivers/mmc/host/omap_hsmmc.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 8900bae..9850280 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -146,6 +146,8 @@ struct omap_hsmmc_host {
 	struct	work_struct	mmc_carddetect_work;
 	void	__iomem		*base;
 	resource_size_t		mapbase;
+	spinlock_t		irq_lock;
+	unsigned long		flags;
 	unsigned int		id;
 	unsigned int		dma_len;
 	unsigned int		dma_sg_idx;
@@ -462,7 +464,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
 	 * enabled, otherwise it is not (i.e. new request).
 	 */
 	if (!in_interrupt())
-		enable_irq(host->irq);
+		spin_unlock_irqrestore(&host->irq_lock, host->flags);
 
 	OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
 	OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
@@ -626,11 +628,14 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
 	struct mmc_data *data;
 	int end_cmd = 0, end_trans = 0, status;
 
+	spin_lock(&host->irq_lock);
+
 	if (host->mrq == NULL) {
 		OMAP_HSMMC_WRITE(host->base, STAT,
 			OMAP_HSMMC_READ(host->base, STAT));
 		/* Flush posted write */
 		OMAP_HSMMC_READ(host->base, STAT);
+		spin_unlock(&host->irq_lock);
 		return IRQ_HANDLED;
 	}
 
@@ -696,6 +701,8 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
 	if ((end_trans || (status & TC)) && host->mrq)
 		omap_hsmmc_xfer_done(host, data);
 
+	spin_unlock(&host->irq_lock);
+
 	return IRQ_HANDLED;
 }
 
@@ -1066,7 +1073,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 	 * retries.
 	 */
 	if (!in_interrupt()) {
-		disable_irq(host->irq);
+		spin_lock_irqsave(&host->irq_lock, host->flags);
 		/*
 		 * Protect the card from I/O if there is a possibility
 		 * it can be removed.
@@ -1085,7 +1092,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 			req->cmd->error = -EBADF;
 			if (req->data)
 				req->data->error = -EBADF;
-			enable_irq(host->irq);
+			spin_unlock_irqrestore(&host->irq_lock, host->flags);
 			mmc_request_done(mmc, req);
 			return;
 		} else if (host->reqs_blocked)
@@ -1100,7 +1107,7 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
 			req->data->error = err;
 		host->mrq = NULL;
 		if (!in_interrupt())
-			enable_irq(host->irq);
+			spin_unlock_irqrestore(&host->irq_lock, host->flags);
 		mmc_request_done(mmc, req);
 		return;
 	}
@@ -1653,6 +1660,7 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
 	mmc->f_max	= 52000000;
 
 	sema_init(&host->sem, 1);
+	spin_lock_init(&host->irq_lock);
 
 	host->iclk = clk_get(&pdev->dev, "ick");
 	if (IS_ERR(host->iclk)) {
-- 
1.5.6.3