[PATCH 2/3] mmc: protect against clockgate races

[PATCH 2/3] mmc: protect against clockgate races

[Linus Walleij]

This adds a mutex to protect from races between gate and ungate
calls colliding.

Cc: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
This should fix the race observed by Ohad earlier.
---
 drivers/mmc/core/host.c  |   11 ++++++++---
 include/linux/mmc/host.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index f74b386..92e3370 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -68,9 +68,9 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 	unsigned long flags;
 
 	if (!freq) {
-		pr_err("%s: frequency set to 0 in disable function, "
-		       "this means the clock is already disabled.\n",
-		       mmc_hostname(host));
+		pr_debug("%s: frequency set to 0 in disable function, "
+			 "this means the clock is already disabled.\n",
+			 mmc_hostname(host));
 		return;
 	}
 	/*
@@ -94,6 +94,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 		spin_unlock_irqrestore(&host->clk_lock, flags);
 		return;
 	}
+	mutex_lock(&host->clk_gate_mutex);
 	spin_lock_irqsave(&host->clk_lock, flags);
 	if (!host->clk_requests) {
 		spin_unlock_irqrestore(&host->clk_lock, flags);
@@ -103,6 +104,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
 		pr_debug("%s: gated MCI clock\n", mmc_hostname(host));
 	}
 	spin_unlock_irqrestore(&host->clk_lock, flags);
+	mutex_unlock(&host->clk_gate_mutex);
 }
 
 /*
@@ -128,6 +130,7 @@ void mmc_host_clk_ungate(struct mmc_host *host)
 {
 	unsigned long flags;
 
+	mutex_lock(&host->clk_gate_mutex);
 	spin_lock_irqsave(&host->clk_lock, flags);
 	if (host->clk_gated) {
 		spin_unlock_irqrestore(&host->clk_lock, flags);
@@ -137,6 +140,7 @@ void mmc_host_clk_ungate(struct mmc_host *host)
 	}
 	host->clk_requests++;
 	spin_unlock_irqrestore(&host->clk_lock, flags);
+	mutex_unlock(&host->clk_gate_mutex);
 }
 
 /**
@@ -214,6 +218,7 @@ static inline void mmc_host_clk_init(struct mmc_host *host)
 	host->clk_gated = false;
 	INIT_WORK(&host->clk_gate_work, mmc_host_clk_gate_work);
 	spin_lock_init(&host->clk_lock);
+	mutex_init(&host->clk_gate_mutex);
 }
 
 /**
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 955bc10..53496bb 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -178,6 +178,7 @@ struct mmc_host {
 	struct work_struct	clk_gate_work; /* delayed clock gate */
 	unsigned int		clk_old;	/* old clock value cache */
 	spinlock_t		clk_lock;	/* lock for clk fields */
+	struct mutex		clk_gate_mutex;	/* mutex for clock gating */
 #endif
 
 	/* host specific block data */
-- 
1.6.3.3

Re: [PATCH 2/3] mmc: protect against clockgate races

[David Vrabel]

Linus Walleij wrote:
> This adds a mutex to protect from races between gate and ungate
> calls colliding.

Can you rework this to not require both host->clk_gate_mutex and
host->clk_lock?  Is host->clk_lock necessary any more?

David

> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
> This should fix the race observed by Ohad earlier.
> ---
>  drivers/mmc/core/host.c  |   11 ++++++++---
>  include/linux/mmc/host.h |    1 +
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index f74b386..92e3370 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -68,9 +68,9 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
>  	unsigned long flags;
>  
>  	if (!freq) {
> -		pr_err("%s: frequency set to 0 in disable function, "
> -		       "this means the clock is already disabled.\n",
> -		       mmc_hostname(host));
> +		pr_debug("%s: frequency set to 0 in disable function, "
> +			 "this means the clock is already disabled.\n",
> +			 mmc_hostname(host));
>  		return;
>  	}
>  	/*
> @@ -94,6 +94,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
>  		spin_unlock_irqrestore(&host->clk_lock, flags);
>  		return;
>  	}
> +	mutex_lock(&host->clk_gate_mutex);
>  	spin_lock_irqsave(&host->clk_lock, flags);
>  	if (!host->clk_requests) {
>  		spin_unlock_irqrestore(&host->clk_lock, flags);
> @@ -103,6 +104,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host)
>  		pr_debug("%s: gated MCI clock\n", mmc_hostname(host));
>  	}
>  	spin_unlock_irqrestore(&host->clk_lock, flags);
> +	mutex_unlock(&host->clk_gate_mutex);
>  }
>  
>  /*
> @@ -128,6 +130,7 @@ void mmc_host_clk_ungate(struct mmc_host *host)
>  {
>  	unsigned long flags;
>  
> +	mutex_lock(&host->clk_gate_mutex);
>  	spin_lock_irqsave(&host->clk_lock, flags);
>  	if (host->clk_gated) {
>  		spin_unlock_irqrestore(&host->clk_lock, flags);
> @@ -137,6 +140,7 @@ void mmc_host_clk_ungate(struct mmc_host *host)
>  	}
>  	host->clk_requests++;
>  	spin_unlock_irqrestore(&host->clk_lock, flags);
> +	mutex_unlock(&host->clk_gate_mutex);
>  }
>  
>  /**
> @@ -214,6 +218,7 @@ static inline void mmc_host_clk_init(struct mmc_host *host)
>  	host->clk_gated = false;
>  	INIT_WORK(&host->clk_gate_work, mmc_host_clk_gate_work);
>  	spin_lock_init(&host->clk_lock);
> +	mutex_init(&host->clk_gate_mutex);
>  }
>  
>  /**
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 955bc10..53496bb 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -178,6 +178,7 @@ struct mmc_host {
>  	struct work_struct	clk_gate_work; /* delayed clock gate */
>  	unsigned int		clk_old;	/* old clock value cache */
>  	spinlock_t		clk_lock;	/* lock for clk fields */
> +	struct mutex		clk_gate_mutex;	/* mutex for clock gating */
>  #endif
>  
>  	/* host specific block data */


-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH 2/3] mmc: protect against clockgate races

[Linus Walleij]

2010/11/16 David Vrabel <david.vrabel@csr.com>:

> Linus Walleij wrote:
>> This adds a mutex to protect from races between gate and ungate
>> calls colliding.
>
> Can you rework this to not require both host->clk_gate_mutex and
> host->clk_lock?  Is host->clk_lock necessary any more?

No I can't. I really wanted to, and the bug Ohad spotted was the
result of trying to do exactly that. But it doesn't work.

The reason is that mmc_host_clk_gate() is called from
mmc_request_done(), which may be called from interrupt
context, thus it cannot take a mutex.

Trying to use only the spinlock gives the inverse problem:
the gating code calls out to mmc_set_ios() or mmc_set_clock()
which must be called in process context because they can be
real slow, and surely shouldn't disable interrupts like taking
a spinlock does.

So what the patch does is handle slowpath and fastpath
alike and for this we need a slowpath synchronization
primitive (the mutex) and a fastpath synchronization
primitive (the spinlock).

Yours,
Linus Walleij

Re: [PATCH 2/3] mmc: protect against clockgate races

[Chris Ball]

Hi Linus,

On Tue, Nov 16, 2010 at 05:56:52PM +0100, Linus Walleij wrote:
> This adds a mutex to protect from races between gate and ungate
> calls colliding.
> 
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>

Thanks, merged into mmc-next for .38.

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child