diff mbox

mmc: tmio: fix recursive spinlock, don't schedule with interrupts disabled

Message ID Pine.LNX.4.64.1106151549050.23363@axis700.grange (mailing list archive)
State Superseded
Headers show

Commit Message

Guennadi Liakhovetski June 15, 2011, 2:06 p.m. UTC
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>
---

Morimoto-san, this should fix the race in TMIO / SDHI driver, that you 
reported here:

http://article.gmane.org/gmane.linux.ports.sh.devel/11349

Please, verify.

 drivers/mmc/host/tmio_mmc.h     |    2 ++
 drivers/mmc/host/tmio_mmc_pio.c |   14 +++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Guennadi Liakhovetski June 15, 2011, 4:50 p.m. UTC | #1
On Wed, 15 Jun 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.

Hm, sorry, please, wait with this one, I have to test it a bit more.

Thanks
Guennadi

> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Morimoto-san, this should fix the race in TMIO / SDHI driver, that you 
> reported here:
> 
> http://article.gmane.org/gmane.linux.ports.sh.devel/11349
> 
> Please, verify.
> 
>  drivers/mmc/host/tmio_mmc.h     |    2 ++
>  drivers/mmc/host/tmio_mmc_pio.c |   14 +++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 8260bc2..b4dfc13 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -73,6 +73,8 @@ struct tmio_mmc_host {
>  
>  	/* Track lost interrupts */
>  	struct delayed_work	delayed_reset_work;
> +	struct work_struct	done;
> +	/* protect host private data */
>  	spinlock_t		lock;
>  	unsigned long		last_req_ts;
>  };
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index ad6347b..1b63045 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -297,10 +297,16 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
>  
>  	host->mrq = NULL;
>  
> -	/* 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 +473,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 +563,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:
> @@ -916,6 +922,7 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
>  
>  	/* 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 +970,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
> 
> --
> 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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 8260bc2..b4dfc13 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -73,6 +73,8 @@  struct tmio_mmc_host {
 
 	/* Track lost interrupts */
 	struct delayed_work	delayed_reset_work;
+	struct work_struct	done;
+	/* protect host private data */
 	spinlock_t		lock;
 	unsigned long		last_req_ts;
 };
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ad6347b..1b63045 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -297,10 +297,16 @@  static void tmio_mmc_finish_request(struct tmio_mmc_host *host)
 
 	host->mrq = NULL;
 
-	/* 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 +473,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 +563,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:
@@ -916,6 +922,7 @@  int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
 
 	/* 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 +970,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);