[PATCH] mmc: omap_hsmmc: Fix conditional locking

[PATCH] mmc: omap_hsmmc: Fix conditional locking

[Thomas Gleixner]

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

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 */

RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Madhusudhan]

> -----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 <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-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Madhusudhan]

> -----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 <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-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Thomas Gleixner]

On Tue, 2 Mar 2010, Madhusudhan 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
> > 
> 
> Can you please elaborate why the existing logic is broken?

Locks are only to be held to serialize data or state. 

The mmc_request_done() call does _NOT_ require that at all. So
dropping the lock there is the right thing to do.

Also conditional locking on in_interrupt() is generally a nono as it
relies on assumptions which are not necessarily true in all
circumstances. Just one simple example: interrupt threading will make
it explode nicely and it did already with the realtime patches
applied.

Such code constructs prevent us to do generic changes to the kernel
behaviour without any real good reason.

> It locks at the new request and unlocks just before issuing the cmd. Further
> IRQ handler has these calls hence the !in_interrupt check.

Aside of the conditional locking I have several issues with that code:

1) The code flow is massively unreadable:

   omap_hsmmc_start_command()
   {
	.....
	if (!in_interrupt())
	   spin_unlock_irq();
   }
 
   omap_hsmmc_request()
   {
	if (!in_interrupt())
	   spin_lock_irq();

	omap_hsmmc_start_command();
   }

We generally want to see the lock/unlock pairs in one function and not
having to figure out where the heck unlock happens.

2) The point of unlocking is patently wrong

   omap_hsmmc_start_command()
   {
	.....
	if (!in_interrupt())
	   spin_unlock_irq();
--->
	OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
--->
        OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
   }

What happens, if you get a spurious interrupt here ? Same for SMP,
though you are probably protected by the core mmc code request
serialization there.

> How does this patch improve that? In fact with your patch for a data
> transfer cmd there are several lock-unlock calls.

1) The patch simply removes conditional locking and moves the lock
   sections to those places which protect something. Aside of that it
   makes the code easier to understand.

2) What's the point of not having those lock/unlocks ? On UP the
   spinlock is a NOOP anyway, so you won't even notice. On SMP you
   won't notice either, simply because the lock is cache hot and
   almost never contended.

Thanks,

	tglx

RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Madhusudhan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, March 03, 2010 4:16 AM
> To: Madhusudhan
> Cc: 'LKML'; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org
> Subject: RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking
> 
> On Tue, 2 Mar 2010, Madhusudhan 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
> > >
> >
> > Can you please elaborate why the existing logic is broken?
> 
> Locks are only to be held to serialize data or state.
> 
> The mmc_request_done() call does _NOT_ require that at all. So
> dropping the lock there is the right thing to do.
> 
> Also conditional locking on in_interrupt() is generally a nono as it
> relies on assumptions which are not necessarily true in all
> circumstances. Just one simple example: interrupt threading will make
> it explode nicely and it did already with the realtime patches
> applied.
> 
> Such code constructs prevent us to do generic changes to the kernel
> behaviour without any real good reason.
> 
> > It locks at the new request and unlocks just before issuing the cmd.
> Further
> > IRQ handler has these calls hence the !in_interrupt check.
> 
> Aside of the conditional locking I have several issues with that code:
> 
> 1) The code flow is massively unreadable:
> 
>    omap_hsmmc_start_command()
>    {
> 	.....
> 	if (!in_interrupt())
> 	   spin_unlock_irq();
>    }
> 
>    omap_hsmmc_request()
>    {
> 	if (!in_interrupt())
> 	   spin_lock_irq();
> 
> 	omap_hsmmc_start_command();
>    }
> 
> We generally want to see the lock/unlock pairs in one function and not
> having to figure out where the heck unlock happens.
> 
> 2) The point of unlocking is patently wrong
> 
>    omap_hsmmc_start_command()
>    {
> 	.....
> 	if (!in_interrupt())
> 	   spin_unlock_irq();
> --->
> 	OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
> --->
>         OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
>    }
> 
> What happens, if you get a spurious interrupt here ? Same for SMP,
> though you are probably protected by the core mmc code request
> serialization there.
> 
> > How does this patch improve that? In fact with your patch for a data
> > transfer cmd there are several lock-unlock calls.
> 
> 1) The patch simply removes conditional locking and moves the lock
>    sections to those places which protect something. Aside of that it
>    makes the code easier to understand.
> 
> 2) What's the point of not having those lock/unlocks ? On UP the
>    spinlock is a NOOP anyway, so you won't even notice. On SMP you
>    won't notice either, simply because the lock is cache hot and
>    almost never contended.
> 

Sounds reasonable.Readbility is still a factor but works for me as the main
intention here is to remove the in_interrupt conditional.

Acked-by: Madhusudhan Chikkature<madhu.cr@ti.com>

Best Regards,
Madhu


> Thanks,
> 
> 	tglx

RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Madhusudhan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, March 03, 2010 4:16 AM
> To: Madhusudhan
> Cc: 'LKML'; linux-omap@vger.kernel.org; linux-mmc@vger.kernel.org
> Subject: RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking
> 
> On Tue, 2 Mar 2010, Madhusudhan 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
> > >
> >
> > Can you please elaborate why the existing logic is broken?
> 
> Locks are only to be held to serialize data or state.
> 
> The mmc_request_done() call does _NOT_ require that at all. So
> dropping the lock there is the right thing to do.
> 
> Also conditional locking on in_interrupt() is generally a nono as it
> relies on assumptions which are not necessarily true in all
> circumstances. Just one simple example: interrupt threading will make
> it explode nicely and it did already with the realtime patches
> applied.
> 
> Such code constructs prevent us to do generic changes to the kernel
> behaviour without any real good reason.
> 
> > It locks at the new request and unlocks just before issuing the cmd.
> Further
> > IRQ handler has these calls hence the !in_interrupt check.
> 
> Aside of the conditional locking I have several issues with that code:
> 
> 1) The code flow is massively unreadable:
> 
>    omap_hsmmc_start_command()
>    {
> 	.....
> 	if (!in_interrupt())
> 	   spin_unlock_irq();
>    }
> 
>    omap_hsmmc_request()
>    {
> 	if (!in_interrupt())
> 	   spin_lock_irq();
> 
> 	omap_hsmmc_start_command();
>    }
> 
> We generally want to see the lock/unlock pairs in one function and not
> having to figure out where the heck unlock happens.
> 
> 2) The point of unlocking is patently wrong
> 
>    omap_hsmmc_start_command()
>    {
> 	.....
> 	if (!in_interrupt())
> 	   spin_unlock_irq();
> --->
> 	OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
> --->
>         OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
>    }
> 
> What happens, if you get a spurious interrupt here ? Same for SMP,
> though you are probably protected by the core mmc code request
> serialization there.
> 
> > How does this patch improve that? In fact with your patch for a data
> > transfer cmd there are several lock-unlock calls.
> 
> 1) The patch simply removes conditional locking and moves the lock
>    sections to those places which protect something. Aside of that it
>    makes the code easier to understand.
> 
> 2) What's the point of not having those lock/unlocks ? On UP the
>    spinlock is a NOOP anyway, so you won't even notice. On SMP you
>    won't notice either, simply because the lock is cache hot and
>    almost never contended.
> 

Sounds reasonable.Readbility is still a factor but works for me as the main
intention here is to remove the in_interrupt conditional.

Acked-by: Madhusudhan Chikkature<madhu.cr@ti.com>

Best Regards,
Madhu


> Thanks,
> 
> 	tglx

Re: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Adrian Hunter]

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
> 
>